cmake: Introduce `LibmultiprocessMacros` module #95

hebasto · 2024-03-15T01:03:25Z

This PR introduces the LibmultiprocessMacros module that is used internally and might be exported as a part of the (future) LibmultiprocessGen package (it is a subject of the follow-up PR).

Also see a discussion in hebasto/bitcoin#118.

hebasto · 2024-03-16T16:12:57Z

Undrafted and aligned with #96.

hebasto · 2024-03-16T16:15:20Z

The PR title and description have been updated.

cmake/LibmultiprocessMacros.cmake

ryanofsky

Code review ACK 45341dc

This looks great! Thanks for writing this. I suggested a documentation comment and a minor simplification above, but would also be happy to merge this as it is. You can let me know what you prefer.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

hebasto · 2024-03-29T11:48:23Z

@ryanofsky

I suggested a documentation comment and a minor simplification above, but would also be happy to merge this as it is. You can let me know what you prefer.

Thank you!

Your suggestions have been implemented.

ryanofsky

Code review ACK 66643d8

Thanks for the update. I think I do want to rename LibmultiprocessMacros.cmake to TargetCapnpSources.cmake so it is named after the function it contains like other cmake modules, but I will do this in a followup

4e70ad4 cmake, refactor: Rename target export files (Hennadii Stepanov) 694b6b1 cmake: Configure `LibmultiprocessGen` package (Hennadii Stepanov) 3b20e35 cmake: Configure `Libmultiprocess` package (Hennadii Stepanov) Pull request description: This PR adds configurations for the `Libmultiprocess` and `LibmultiprocessGen` packages. The Bitcoin Core branch, with uses this PR branch, is available [here](https://github.com/hebasto/bitcoin/tree/240329-cmake-CI-mpgen). As a suggestion for a follow-up, it seems worth disabling `mpgen` target for cross-compiling (see bitcoin/bitcoin#29665 (comment) and CMake [docs](https://cmake.org/cmake/help/book/mastering-cmake/chapter/Cross%20Compiling%20With%20CMake.html#running-executables-built-in-the-project)). Based on #95. ACKs for top commit: ryanofsky: Code review ACK 4e70ad4. This should make it easier to depend on libmultprocess in cmake projects, using a find_package call and not having to include files directly. Tree-SHA512: 231d00fd3b03bff40d010d39636a4c6b26e071a1d2c220f204ef536d6f3f7b3c81d2d05a16e3bb23f405a45e2b763cd88344cabd9eb407f1967b766b328815a4

…and chaincodelabs#96 Rename "LibmultiprocessMacros.cmake" module to "TargetCapnpSources.cmake" to match the name of the target_capnp_sources function it contains. Also install it <prefix>/lib/cmake/Libmultiprocess/TargetCapnpSources.cmake instead of: <prefix>/lib/cmake/LibmultiprocessGen/LibmultiprocessMacros.cmake Rename the "LibmultiprocessGen" package to "LibmultiprocessBin" and rename the "Libmultiprocess" package to "LibmultiprocessLib", so package names are consistent with component names "bin" and "lib", and one package name is not a prefix of the other. Also rename intermediate files to match component names. |----------------+----------------------------------+----------------------------------------------| | | New | Current | |----------------+----------------------------------+----------------------------------------------| | Component name | lib | lib | | Package name | LibmultiprocessLib | Libmultiprocess | | Config file | LibmultiprocessLibConfig.cmake | Libmultiprocess/LibmultiprocessConfig.cmake | | Targets file | Libmultiprocess/LibTargets.cmake | Libmultiprocess/LibmultiprocessTargets.cmake | |----------------+----------------------------------+----------------------------------------------| |----------------+----------------------------------+----------------------------------------------------| | | New | Current | |----------------+----------------------------------+----------------------------------------------------| | Component name | bin | bin | | Package name | LibmultiprocessBin | LibmultiprocessGen | | Config file | LibmultiprocessBinConfig.cmake | LibmultiprocessGen/LibmultiprocessGenConfig.cmake | | Targets file | Libmultiprocess/BinTargets.cmake | LibmultiprocessGen/LibmultiprocessGenTargets.cmake | |----------------+----------------------------------+----------------------------------------------------|

…#96 c6a1d7f cmake: rename new packages and module introduced in #95 and #96 (Ryan Ofsky) Pull request description: Rename "LibmultiprocessMacros.cmake" module introduced in #95 to "TargetCapnpSources.cmake" to match the name of the `target_capnp_sources` function it contains. Also install it to: ```sh <prefix>/lib/cmake/Libmultiprocess/TargetCapnpSources.cmake ``` instead of: ```sh <prefix>/lib/cmake/LibmultiprocessGen/LibmultiprocessMacros.cmake ``` Rename the "Libmultiprocess" and "LibmultiprocessGen" packages introduced in #96 to "LibmultiprocessLib" and "LibmultiprocessBin", so package names are consistent with component names "lib" and "bin", and one package name is not a prefix of the other. Also rename intermediate files to match component names. | | New | Current | |----------------|----------------------------------|----------------------------------------------| | Component name | lib | lib | | Package name | LibmultiprocessLib | Libmultiprocess | | Config file | LibmultiprocessLibConfig.cmake | Libmultiprocess/LibmultiprocessConfig.cmake | | Targets file | Libmultiprocess/LibTargets.cmake | Libmultiprocess/LibmultiprocessTargets.cmake | | | New | Current | |----------------|----------------------------------|----------------------------------------------------| | Component name | bin | bin | | Package name | LibmultiprocessBin | LibmultiprocessGen | | Config file | LibmultiprocessBinConfig.cmake | LibmultiprocessGen/LibmultiprocessGenConfig.cmake | | Targets file | Libmultiprocess/BinTargets.cmake | LibmultiprocessGen/LibmultiprocessGenTargets.cmake | Top commit has no ACKs. Tree-SHA512: db099f34e14f5433c644190359bef0f3b836401b31c8eea3c4f7d732ec06402eb01063360d8b7ebbed9522be9c6758e157e46e15cafd80ac2bebc5c915ad2160

hebasto · 2024-03-30T05:06:39Z

I think I do want to rename LibmultiprocessMacros.cmake to TargetCapnpSources.cmake so it is named after the function it contains like other cmake modules, but I will do this in a followup

FWIW, when I chose the name, I followed the wide accepted practice for naming a CMake file, which provides the package-specific functions. For example, https://packages.ubuntu.com/jammy/amd64/qtbase5-dev/filelist

ryanofsky · 2024-03-30T15:18:10Z

(EDIT: This comment was actually posted in response to #97 (comment) in the other issue)

Oops, sorry, I saw the <prefix>/(cmake|CMake)/ entry in that table and thought just putting the files in the cmake/ directory would work. I didn't realize that entry does not include the "lib" component though. I guess the only way to fix this is to repeat the package name twice in installed paths, which seems ugly to me but does seem to be the cmake convention as you pointed out #95 (comment)

I do want package names to be consistent with component names, though. Will post a followup adding directory prefixes.

Fix "connection: run async cleanups in LIFO not FIFO order" chaincodelabs/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: chaincodelabs/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module chaincodelabs/libmultiprocess#96: cmake: Introduce packages chaincodelabs/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 chaincodelabs/libmultiprocess#98: cmake: Combine installed packages chaincodelabs/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print chaincodelabs/libmultiprocess#100: doc: Add various code comments and documentation chaincodelabs/libmultiprocess#102: doc: Document shutdown sequences better

depends: Update libmultiprocess library Fix "connection: run async cleanups in LIFO not FIFO order" chaincodelabs/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: chaincodelabs/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module chaincodelabs/libmultiprocess#96: cmake: Introduce packages chaincodelabs/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 chaincodelabs/libmultiprocess#98: cmake: Combine installed packages chaincodelabs/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print chaincodelabs/libmultiprocess#100: doc: Add various code comments and documentation chaincodelabs/libmultiprocess#102: doc: Document shutdown sequences better

Fix "connection: run async cleanups in LIFO not FIFO order" chaincodelabs/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: chaincodelabs/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module chaincodelabs/libmultiprocess#96: cmake: Introduce packages chaincodelabs/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 chaincodelabs/libmultiprocess#98: cmake: Combine installed packages chaincodelabs/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print chaincodelabs/libmultiprocess#100: doc: Add various code comments and documentation chaincodelabs/libmultiprocess#102: doc: Document shutdown sequences better

hebasto force-pushed the 240314-macro branch from aeb03f0 to 45341dc Compare March 16, 2024 16:12

hebasto marked this pull request as ready for review March 16, 2024 16:12

hebasto mentioned this pull request Mar 16, 2024

cmake: Introduce packages #96

Merged

hebasto changed the title ~~cmake: Introduce MpgenMacros module~~ cmake: Introduce LibmultiprocessMacros module Mar 16, 2024

ryanofsky self-assigned this Mar 19, 2024

ryanofsky reviewed Mar 28, 2024

View reviewed changes

cmake/LibmultiprocessMacros.cmake Outdated Show resolved Hide resolved

ryanofsky approved these changes Mar 28, 2024

View reviewed changes

hebasto and others added 3 commits March 29, 2024 11:46

cmake: Add LibmultiprocessMacros module

d9ec22f

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

cmake, refactor: Use target_capnp_sources for mptest target

bd2dfe2

cmake, refactor: Use target_capnp_sources for examples

66643d8

hebasto force-pushed the 240314-macro branch from 45341dc to 66643d8 Compare March 29, 2024 11:47

ryanofsky approved these changes Mar 29, 2024

View reviewed changes

ryanofsky merged commit 19dea85 into chaincodelabs:master Mar 29, 2024

ryanofsky mentioned this pull request Mar 30, 2024

cmake: rename new packages and module introduced in #95 and #96 #97

Merged

hebasto deleted the 240314-macro branch March 30, 2024 05:24

ryanofsky mentioned this pull request Apr 1, 2024

cmake: Combine installed packages #98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Introduce `LibmultiprocessMacros` module #95

cmake: Introduce `LibmultiprocessMacros` module #95

hebasto commented Mar 15, 2024 •

edited

Loading

hebasto commented Mar 16, 2024

hebasto commented Mar 16, 2024

ryanofsky left a comment

hebasto commented Mar 29, 2024

ryanofsky left a comment

hebasto commented Mar 30, 2024

ryanofsky commented Mar 30, 2024 •

edited

Loading

cmake: Introduce LibmultiprocessMacros module #95

cmake: Introduce LibmultiprocessMacros module #95

Conversation

hebasto commented Mar 15, 2024 • edited Loading

hebasto commented Mar 16, 2024

hebasto commented Mar 16, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

hebasto commented Mar 29, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

hebasto commented Mar 30, 2024

ryanofsky commented Mar 30, 2024 • edited Loading

cmake: Introduce `LibmultiprocessMacros` module #95

cmake: Introduce `LibmultiprocessMacros` module #95

hebasto commented Mar 15, 2024 •

edited

Loading

ryanofsky commented Mar 30, 2024 •

edited

Loading